Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modify counting for search limit in typeahead (patching it locally) #1369

Merged
merged 1 commit into from
Sep 29, 2022

Conversation

kinow
Copy link
Collaborator

@kinow kinow commented Sep 28, 2022

Reasons for creating this PR

Link to relevant issue(s), if any

Description of the changes in this PR

Monkey patching typeahead to simulate as if twitter/typeahead.js#1774 had been merged.

Known problems or uncertainties in this PR

It's hard to write a test for this change, sorry. Only way to review and verify is really deploying somewhere with YSO to compare the search results.

Checklist

  • phpUnit tests pass locally with my changes
  • I have added tests that show that the new code works, or tests are not relevant for this PR (e.g. only HTML/CSS changes)
  • The PR doesn't reduce accessibility of the front-end code (e.g. tab focus, scaling to different resolutions, use of .sr-only class, color contrast)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

@kinow
Copy link
Collaborator Author

kinow commented Sep 28, 2022

@osma if this works I'll have level-up'ed my monkey-patching in JS (nothing to be proud of, unfortunately 😅).

Cheers
Bruno

@sonarcloud
Copy link

sonarcloud bot commented Sep 28, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.8% 0.8% Duplication

@codecov
Copy link

codecov bot commented Sep 28, 2022

Codecov Report

Base: 71.17% // Head: 71.17% // No change to project coverage 👍

Coverage data is based on head (c237f01) compared to base (d9c6034).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #1369   +/-   ##
=========================================
  Coverage     71.17%   71.17%           
  Complexity     1650     1650           
=========================================
  Files            32       32           
  Lines          3788     3788           
=========================================
  Hits           2696     2696           
  Misses         1092     1092           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kinow kinow marked this pull request as ready for review September 28, 2022 19:11
@kinow kinow changed the title Monkey-patch typeahead using patch from their PR 1774 Modify counting for search limit in typeahead (patching it locally) Sep 28, 2022
@osma
Copy link
Member

osma commented Sep 29, 2022

Whoa, this is awesome @kinow ! You sure have earned a 🏅 for this feat!

I tested this and it seems to work, at least for the known example cases. There's no way for me to tell whether this will cause any new troubles, but having to choose between known-buggy and possibly-buggy-but-seems-to-work, I think the latter is a much better option.

It's clear we need to replace Typeahead.js, and I think this will be done rather soon as we are going to rebuild the UI for 3.0 anyway. But this solves the problem for now so I'll merge it! 👍

@osma osma merged commit 0bda0f5 into NatLibFi:master Sep 29, 2022
@osma osma added the bug label Sep 29, 2022
@osma osma added this to the 2.16 milestone Sep 29, 2022
@kinow kinow deleted the monkey-patching-level-up branch September 29, 2022 08:28
@kinow
Copy link
Collaborator Author

kinow commented Sep 29, 2022

Whoa, this is awesome @kinow ! You sure have earned a medal_sports for this feat!

🎉 hooray!

I tested this and it seems to work, at least for the known example cases. There's no way for me to tell whether this will cause any new troubles, but having to choose between known-buggy and possibly-buggy-but-seems-to-work, I think the latter is a much better option.

👍 agreed.

It's clear we need to replace Typeahead.js, and I think this will be done rather soon as we are going to rebuild the UI for 3.0 anyway. But this solves the problem for now so I'll merge it! +1

Writing from scratch is a little tricky, but if there's some reactive framework (react/vue/svelte/or even those smaller micro frameworks) it could save a few lines.

Thanks @osma!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search box autocomplete does not give a full list of results
2 participants